Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Added test of type checking and autoargs #71

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

dpgrote
Copy link
Member

@dpgrote dpgrote commented Mar 16, 2022

This is an alternative proposal to what PR #63 is doing. This set of changes is much smaller but still does all of the things desired.

  • Reduce the number of times parameters are referenced
  • Runtime type checking on scalars and vectors
  • Checks lengths of scalars
  • Handles keyword arguments as before (allowing codename_arg with checking)
  • Fully backwards compatible (and allows class to be updated one by one)
  • Allows argument value checking in the __init__ routine or separate callable method
  • The type checking has been added to all classes

Units tests need to be added, but there isn't much to test since it is using commonly used packages.
Documentation can be added (much of that from PR #63 can be carried over).

@dpgrote dpgrote changed the title Added test of type checking and autoargs [WIP]Added test of type checking and autoargs Mar 16, 2022
@dpgrote dpgrote mentioned this pull request Mar 16, 2022
6 tasks
@@ -1,2 +1,4 @@
numpy~=1.15
scipy~=1.5
autoclass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -1,2 +1,4 @@
numpy~=1.15
scipy~=1.5
autoclass
typeguard
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ax3l ax3l changed the title [WIP]Added test of type checking and autoargs [WIP] Added test of type checking and autoargs Mar 16, 2022
Bx_expression : Expression = None,
By_expression : Expression = None,
Bz_expression : Expression = None,
lower_bound : VectorFloat3 = [None,None,None],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is assigning a list as default argument value (old and new) actually possible?

Common Python bug:
https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this code works as is, and I am aware of the issue. In this case, however, it is not expected that the argument, lower_bound, will be modified, but only read. An alternative here would be to have lower_bound default to None. This would be more consistent with other vectors that default to None. In this PR, I wanted to avoid changing the interface, but the change can be made.

@s9105947
Copy link

I like the short form very much, this also gets very close to the python dataclass notation which performs almost the same by specifying class attributes.

The following details raised my attention:

  • Do we need developer-overwriteable __init__() methods? I am worried about the possibility to simply specify arbitrary code -- this would undermine the predictability of PICMI objects.
  • How are mandatory attributes treated?
  • What is the difference between a default argument of the wrong type, and a user-supplied argument of the default type.
  • How is this interface used to extend existing PICMI classes in PICMI implementations?

These are design principles, I've sent you an email for a short meeting on the matter to put these design principles into writing.

@dpgrote
Copy link
Member Author

dpgrote commented Mar 17, 2022

Thanks for looking at this PR. Here are comments on the issues:

  • Regarding __init__, this is where any checks can be done. I don't think there is an issue with adding code to the __init__ routine (at the picmi level). Remember that picmi is monitored and that any changes would need to be approved. For the implementations, the standard would be that the implementation not overwrite the __init__ routine but only add the init method to do what is needed.
  • Mandatory arguments are setup the same way but just without a default value, e.g. n_physical_particles : int, .
  • I checked this. The default argument does not need to have the specified type (so None is ok). If the user sets the argument to the default value, it is allowed. This is ok, since there is no way to tell the difference.
  • The changes here don't require any changes to the implementation (from what it was before).

@ax3l
Copy link
Member

ax3l commented Mar 17, 2022

That sounds great to me.

I would probably still introduce a check member function as we have also the init member function.
That way, we can check physical ranges both in the constructor (or init) as well as before we call the step() or inputs export, respectively. And thus, we can check parameters that users changed in between, e.g., though custom setters, their own properties, or accessing a member variable directly after construction.

@dpgrote
Copy link
Member Author

dpgrote commented Mar 17, 2022

This check method can be added, but I would suggest that this should be done in a separate PR.

Once the changes here are accepted, I would suggest that this PR be merged. The other classes can be updated to use the new features in separate PRs (to keep the PRs small). This also allows the work to be spread out (since people are busy).

s9105947
s9105947 previously approved these changes Mar 21, 2022

VectorFloat3 = typing.NewType('VectorFloat3', typing.Union[Sequence[float], np.ndarray[3, np.float64]])
VectorInt3 = typing.NewType('VectorInt3', typing.Union[Sequence[int], np.ndarray[3, np.int64]])
Expression = typing.NewType('Expression', str)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you fell about moving this into a separate namespace (for python a module i.e. here a file)?

This way every type will get a prefix and the distinction between python native & PICMI custom (and perhaps other types) will become clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was considering that, so that all of the type declarations are in the same place. It is somewhat tricky though because of the circular imports that will happen since some of the types depend on PICMI classes which will depend on the types. I'll see what I can put together.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had that problem too, PEP 484 allows using strings instead of literals for forward declarations

But ultimately your call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks, using strings worked. I am clearly not the first person to come across this circular definition problem :)


SpeciesType = typing.NewType('SpeciesType', typing.Union[particles.PICMI_Species,
particles.PICMI_MultiSpecies])
SpeciesArgument = typing.NewType('SpeciesArgument', typing.Union[SpeciesType, Sequence[SpeciesType]])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above: How do you feel about collecting all type definitions in a single file?

I am worried that existing type definitions might be overlooked and defined over and over again.

@dpgrote
Copy link
Member Author

dpgrote commented Mar 25, 2022

I've implemented the type checking in all of the classes. All of the defined types are not setup in the picmi_types.py files. Though, there is some kludginess there because of the way the type definitions that are in strings are evaluated. For example, for each type that is used, a module must import the modules used in the type definition (that's why particles.py must import fields for instance).

The problem comes when a type is used that depends on the same module where the type is used. In this case, the module can't import itself, so the type string cannot reference the module. For this reason, I had to define two versions of the GridType, with and without the fields references

An alternative is to do "*" imports, i.e. particles could do "from .fields import *" (so the fields prefix in the type definition is not needed), but this has its own messiness. I'm receptive to other alternative ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants